ci: Batch wheels by package name - #135
Conversation
Signed-off-by: David Gardner <dagardner@nvidia.com>
Walkthrough
ChangesKitMaker release aggregation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci/artifactory_upload.py`:
- Around line 56-64: Update the coverage validation near the published-wheels
batching flow to compare project IDs with unique package names, not individual
entries in published_wheels. Use the grouped packages mapping (or validate
set(packages) against project_ids.keys()) so multiple wheels for one package do
not produce a false warning, while preserving the existing warning behavior for
genuinely missing projects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9c64effc-6eef-4b99-bf99-5b837cdfa8e6
📒 Files selected for processing (1)
scripts/ci/artifactory_upload.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test (Python 3.14, windows-amd64)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
scripts/ci/artifactory_upload.py
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
scripts/ci/artifactory_upload.py
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.
**/*.{rs,py}: When changing the Rust core or public schemas, run both the Rust and Python test suites.
When adding functionality, include tests in the corresponding Rust crate or the relevant area undertests/.
Files:
scripts/ci/artifactory_upload.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.
Files:
scripts/ci/artifactory_upload.py
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
Files:
scripts/ci/artifactory_upload.py
**/*.{py,pyi,rs}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For Python SDK or PyO3 binding changes, use
python-tests, run focused pytest tests first, thenjust test-python; rebuild withjust build-pythonwhen native code or packaging changes.
Files:
scripts/ci/artifactory_upload.py
**/*.{rs,py,toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
When editing version helpers, verify every
nemo-fabric-*workspace package through Cargo metadata and reject a static version inpython/pyproject.toml.
Files:
scripts/ci/artifactory_upload.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
scripts/ci/artifactory_upload.py
**/*.{rs,py,html,md,mdx,toml,yml,yaml,sh,bash}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Rust, Python, HTML, Markdown, MDX, TOML, YAML, and shell source files must include the project SPDX copyright and Apache-2.0 license headers using the comment syntax appropriate to each file type.
Files:
scripts/ci/artifactory_upload.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use type annotations for public Python APIs.
Files:
scripts/ci/artifactory_upload.py
**/*.{py,rs}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{py,rs}: Keep native Python binding declarations synchronized with their Rust implementations.
Usesnake_casefor functions and variables; usePascalCasefor Rust types and Python classes.
Files:
scripts/ci/artifactory_upload.py
🔇 Additional comments (2)
scripts/ci/artifactory_upload.py (2)
7-7: LGTM!
63-75: 🎯 Functional CorrectnessAdd regression coverage for package-level batching.
Verify that two wheels for one package produce exactly one POST containing both URLs, while different packages produce separate POSTs. As per coding guidelines, added functionality should include relevant tests, and Python changes require
just test-python.Source: Coding guidelines
Signed-off-by: David Gardner <dagardner@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/ci/artifactory_upload.py (1)
51-85: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd regression coverage for grouped release payloads.
No test change accompanies this new aggregation behavior. Mock KitMaker requests and wheel metadata to verify one POST per package, all wheel URLs in that package’s payload, and no POSTs when a package project is missing. Run focused pytest first, then
just test-python.As per coding guidelines, “When adding functionality, include tests in the corresponding Rust crate or the relevant area under
tests/,” and Python changes requirejust test-python.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/artifactory_upload.py` around lines 51 - 85, Add regression tests for the grouped release flow around the package aggregation and POST logic: mock KitMaker requests and wheel metadata, verify exactly one POST per package with every grouped wheel URL in its payload, and verify no POST occurs when a package project is missing. Run the focused pytest coverage first, then run just test-python.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/ci/artifactory_upload.py`:
- Around line 51-85: Add regression tests for the grouped release flow around
the package aggregation and POST logic: mock KitMaker requests and wheel
metadata, verify exactly one POST per package with every grouped wheel URL in
its payload, and verify no POST occurs when a package project is missing. Run
the focused pytest coverage first, then run just test-python.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ea57b285-050b-4c77-83ac-4c223cf892b3
📒 Files selected for processing (1)
scripts/ci/artifactory_upload.py
📜 Review details
⏰ Context from checks skipped due to timeout. (17)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.11, linux-arm64)
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.11, linux-amd64)
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.12, linux-amd64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
scripts/ci/artifactory_upload.py
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
scripts/ci/artifactory_upload.py
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.
**/*.{rs,py}: When changing the Rust core or public schemas, run both the Rust and Python test suites.
When adding functionality, include tests in the corresponding Rust crate or the relevant area undertests/.
Files:
scripts/ci/artifactory_upload.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.
Files:
scripts/ci/artifactory_upload.py
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
Files:
scripts/ci/artifactory_upload.py
**/*.{py,pyi,rs}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For Python SDK or PyO3 binding changes, use
python-tests, run focused pytest tests first, thenjust test-python; rebuild withjust build-pythonwhen native code or packaging changes.
Files:
scripts/ci/artifactory_upload.py
**/*.{rs,py,toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
When editing version helpers, verify every
nemo-fabric-*workspace package through Cargo metadata and reject a static version inpython/pyproject.toml.
Files:
scripts/ci/artifactory_upload.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
scripts/ci/artifactory_upload.py
**/*.{rs,py,html,md,mdx,toml,yml,yaml,sh,bash}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Rust, Python, HTML, Markdown, MDX, TOML, YAML, and shell source files must include the project SPDX copyright and Apache-2.0 license headers using the comment syntax appropriate to each file type.
Files:
scripts/ci/artifactory_upload.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use type annotations for public Python APIs.
Files:
scripts/ci/artifactory_upload.py
**/*.{py,rs}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{py,rs}: Keep native Python binding declarations synchronized with their Rust implementations.
Usesnake_casefor functions and variables; usePascalCasefor Rust types and Python classes.
Files:
scripts/ci/artifactory_upload.py
🔇 Additional comments (2)
scripts/ci/artifactory_upload.py (2)
57-61: Validate package names, not project counts.This remains unresolved: equal counts can still omit a required package, causing a
KeyErrorat Line 65—potentially after other package releases were posted. Compute missing package names and fail before the POST loop.Proposed fix
- if len(projects) < len(packages): - print( - f"Warning: KitMaker returned {len(projects)} projects for {len(packages)} packages.", - flush=True, - ) + missing_packages = sorted(set(packages).difference(project_ids)) + if missing_packages: + raise RuntimeError( + "KitMaker is missing projects for packages: " + + ", ".join(missing_packages) + )
7-7: LGTM!
|
/merge |
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Closes FABRIC-146
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.
Summary by CodeRabbit